-
Notifications
You must be signed in to change notification settings - Fork 280
added {model: string;}
arg to client.connect()
#112
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
As it was, the library did not allow setting the model used by the realtime API. This commit should fix that by making the model a mandatory argument of the `client.connect` method. The model must be passed to `connect` as a string property called `model` in an object.
Oh sorry about that, hadn't noticed.
Thanks for the review.
I had the library already added to my project
as a submodule so I performed the changes
directly from the submodule as it was very
convenient for testing.
Then tsc must have somehow picked up the type specialization from the
parent project I suppose?
P.S.: I'm replying to the e-mail notification, I have no idea if and where
this will appear on GitHub, couldn't find the comment on the website.
…On Thu, May 8, 2025, 22:23 iemesowum ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In dist/lib/conversation.d.ts
<#112 (comment)>
:
> @@ -66,7 +66,7 @@ export class RealtimeConversation {
'response.audio.delta': (event: any) => {
item: any;
delta: {
- audio: Int16Array;
+ audio: Int16Array<ArrayBuffer>;
Minor comment: this doesn't appear to be related to the main change.
—
Reply to this email directly, view it on GitHub
<#112 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AALESE7UDU3WE3CYMZKAO7D25O4KNAVCNFSM6AAAAAB4NC5NQSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDQMRWGI3TQMZWHE>
.
You are receiving this because you authored the thread.
Message ID: <openai/openai-realtime-api-beta/pull/112/review/2826278369@
github.com>
|
I deleted my comment. 😄 It still applies, but it's okay. I also got the workaround here working from the relay server: ...after applying the patch to use the passed in model. EDIT: which you also fixed, of course: https://github.com/openai/openai-realtime-api-beta/pull/112/files#diff-3814d8caba69e5fdf3b23eed56ec7d708b8ede5aed6d686ac579421877938ec7R116. |
Ahah I get it.
Yes, there are probably something like 30 open PR's on that subject. All of
them with small variations, you can pick the one you like the most lol.
I think everyone is assuming that it's such an obvious fix that there's no
need to check if someone has already proposed it before implementing it,
thinking something like "if it had been proposed, it would be on `main`
already".
At least that was more or less my thought process.
…On Thu, May 8, 2025, 23:30 iemesowum ***@***.***> wrote:
*iemesowum* left a comment (openai/openai-realtime-api-beta#112)
<#112 (comment)>
Oh sorry about that, hadn't noticed. Thanks for the review. I had the
library already added to my project as a submodule so I performed the
changes directly from the submodule as it was very convenient for testing.
Then tsc must have somehow picked up the type specialization from the
parent project I suppose? P.S.: I'm replying to the e-mail notification, I
have no idea if and where this will appear on GitHub, couldn't find the
comment on the website.
… <#m_-2434988027394151577_>
On Thu, May 8, 2025, 22:23 iemesowum *@*.*> wrote: @.** commented on this
pull request. ------------------------------ In dist/lib/conversation.d.ts <#112
(comment)
<#112 (comment)>>
: > @@ -66,7 +66,7 @@ export class RealtimeConversation {
'response.audio.delta': (event: any) => { item: any; delta: { - audio:
Int16Array; + audio: Int16Array; Minor comment: this doesn't appear to be
related to the main change. — Reply to this email directly, view it on
GitHub <#112 (review)
<#112 (review)>>,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AALESE7UDU3WE3CYMZKAO7D25O4KNAVCNFSM6AAAAAB4NC5NQSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDQMRWGI3TQMZWHE
. You are receiving this because you authored the thread. Message ID: <
/pull/112 <#112>
/review/2826278369@ github.com>
I deleted my comment. 😄
It still applies, but it's okay.
I also got the workaround here working from the relay server:
#89 (comment)
<#89 (comment)>
After applying this patch to use the passed in model.
—
Reply to this email directly, view it on GitHub
<#112 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AALESE4W3FONE2KEQKSVMJT25PEIHAVCNFSM6AAAAAB4NC5NQSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDQNRUGM3DGNJZG4>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
As it was, the library did not allow setting the
model used by the realtime API.
This commit should fix that by making the model
a mandatory argument of the
client.connect
method.The model must be passed to
connect
as a string property calledmodel
in an object.E.g.
I've made the argument mandatory because if you use the wrong model your wallet can suffer quickly.
The
mini
model is like 10x less expensive. Just sayin'...